Skip to content

refactor: extract SnapshotController so the runtime no longer brokers /undo#2707

Merged
dgageot merged 1 commit intodocker:mainfrom
dgageot:board/a8bb59ca7abad016
May 9, 2026
Merged

refactor: extract SnapshotController so the runtime no longer brokers /undo#2707
dgageot merged 1 commit intodocker:mainfrom
dgageot:board/a8bb59ca7abad016

Conversation

@dgageot
Copy link
Copy Markdown
Member

@dgageot dgageot commented May 7, 2026

Fixes #2701.

The runtime used to know about snapshots: it carried snapshotsEnabled, exposed UndoLastSnapshot / ListSnapshots / ResetSnapshot as methods on LocalRuntime, and the TUI commands reached snapshots through that runtime. This PR moves snapshots into a SnapshotController returned by builtins.RegisterSnapshot, threaded into both the runtime (as an AutoInjector) and the App.

What changed

  • pkg/hooks/builtins/snapshot.go \u2014 new SnapshotController interface (Enabled, UndoLast, List, Reset) and RegisterSnapshot(r, enabled) (SnapshotController, error) entry point that registers the snapshot builtin and returns a controller. The controller's AutoInject mounts the snapshot hook on session/turn boundaries when enabled.
  • pkg/hooks/builtins/builtins.go \u2014 added a generic AutoInjector interface; Register no longer wires the snapshot builtin and AgentDefaults.Snapshot is gone (the snapshot block in ApplyAgentDefaults moved onto the controller).
  • pkg/runtime/snapshot.go \u2014 deleted.
  • pkg/runtime/runtime.go / hooks.go \u2014 removed WithSnapshots, snapshotsEnabled, and the snapshot methods. New WithHooksRegistry lets the embedder ship a registry with snapshot pre-registered, and WithAutoInjector plumbs in any AutoInjector. buildHooksExecutors runs the registered injectors.
  • pkg/app/ \u2014 App gains a snapshotController via WithSnapshotController(c). UndoLastSnapshot / ListSnapshots / ResetSnapshot delegate directly to the controller, resolving cwd from the session (with os.Getwd fallback).
  • cmd/root/run.go \u2014 a small snapshotRuntimeOpts helper builds a registry, calls RegisterSnapshot, and returns the runtime opts plus the controller; buildAppOpts threads the controller into the App. The session spawner does the same so each spawned session has independent snapshot state.
  • lint/hook_builtins_registered.go \u2014 exempts snapshot.go from the "must be registered in builtins.go" check since it has its own RegisterSnapshot entry point.

Acceptance criteria

  • pkg/runtime/snapshot.go is gone.
  • LocalRuntime exposes no snapshot-specific methods.
  • TUI commands drive the controller directly (through the App, no longer through the runtime).
  • WithSnapshots is removed from runtime.Opt; configuration moves to the controller.

Validation

  • task build \u2014 ok
  • go test ./... \u2014 ok
  • task lint \u2014 0 issues

@dgageot dgageot requested a review from a team as a code owner May 7, 2026 16:00
@aheritier aheritier added kind/refactor PR refactors code without behavior change area/agent For work that has to do with the general agent loop/agentic features of the app effort:medium Multiple files or components, some design decisions needed go Pull requests that update go code labels May 7, 2026
@docker-agent
Copy link
Copy Markdown

PR Review Failed — The review agent encountered an error and could not complete the review. View logs.

@docker-agent
Copy link
Copy Markdown

PR Review Failed — The review agent encountered an error and could not complete the review. View logs.

@rumpl
Copy link
Copy Markdown
Member

rumpl commented May 9, 2026

This PR has merge conflicts with main. Could you rebase? Happy to review once it's clean.

@dgageot dgageot force-pushed the board/a8bb59ca7abad016 branch from 5426117 to dfd5992 Compare May 9, 2026 07:24
rumpl
rumpl previously approved these changes May 9, 2026
Copy link
Copy Markdown
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

… /undo

Drops WithSnapshots in favour of builtins.RegisterSnapshot returning a
controller the embedder threads into both the runtime (as an
AutoInjector) and the App. LocalRuntime no longer carries snapshot
methods, and pkg/runtime/snapshot.go is gone.

Fixes docker#2701
@dgageot dgageot merged commit b657530 into docker:main May 9, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/agent For work that has to do with the general agent loop/agentic features of the app effort:medium Multiple files or components, some design decisions needed go Pull requests that update go code kind/refactor PR refactors code without behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(hooks): extract SnapshotController and stop the runtime from knowing snapshots exist

4 participants